Skip to content

gh-151039: Fix a crash when _datetimetypes outlive _datetime module#151044

Merged
sobolevn merged 9 commits into
python:mainfrom
sobolevn:issue-151039
Jun 9, 2026
Merged

gh-151039: Fix a crash when _datetimetypes outlive _datetime module#151044
sobolevn merged 9 commits into
python:mainfrom
sobolevn:issue-151039

Conversation

@sobolevn

@sobolevn sobolevn commented Jun 7, 2026

Copy link
Copy Markdown
Member

Several changes:

  1. Always check that GET_CURRENT_STATE can return NULL
  2. Unwrap (void)PyWeakref_GetRef call into a full check for safety, there are multiple things that can go wrong when getting a weakref
  3. Remove assert(!PyErr_Occurred()); followed by if (PyErr_Occurred()) {

@sobolevn sobolevn requested a review from picnixz June 7, 2026 14:33
@sobolevn sobolevn added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes needs backport to 3.15 pre-release feature fixes, bugs and security fixes labels Jun 7, 2026
@sobolevn

sobolevn commented Jun 7, 2026

Copy link
Copy Markdown
Member Author

Ok, I just discovered #132413 which is related.

Comment thread Lib/test/datetimetester.py Outdated
Comment thread Misc/NEWS.d/next/Library/2026-06-07-17-29-33.gh-issue-151039.AZ0qBn.rst Outdated
…Z0qBn.rst

Co-authored-by: Stan Ulbrych <stan@python.org>
Comment thread Lib/test/datetimetester.py Outdated
Comment thread Lib/test/datetimetester.py Outdated
Comment thread Modules/_datetimemodule.c Outdated
@sobolevn sobolevn changed the title gh-151039: Fix a crash when datetime.timedelta outlives _datetime module gh-151039: Fix a crash when _datetimetypes outlive _datetime module Jun 8, 2026
@vstinner

vstinner commented Jun 8, 2026

Copy link
Copy Markdown
Member

Can you change get_current_module() to return an integer? -1 on error, 0 if the module is NULL, 1 on success (module is not NULL). It would avoid having to call PyErr_Occurred() at each call. Example of patch:

Details
diff --git a/Modules/_datetimemodule.c b/Modules/_datetimemodule.c
index eb05539d4c2..efc81538e38 100644
--- a/Modules/_datetimemodule.c
+++ b/Modules/_datetimemodule.c
@@ -126,8 +126,8 @@ get_module_state(PyObject *module)
 
 #define INTERP_KEY ((PyObject *)&_Py_ID(cached_datetime_module))
 
-static PyObject *
-get_current_module(PyInterpreterState *interp)
+static int
+get_current_module(PyInterpreterState *interp, PyObject **p_mod)
 {
     PyObject *mod = NULL;
 
@@ -151,11 +151,14 @@ get_current_module(PyInterpreterState *interp)
             Py_DECREF(ref);
         }
     }
-    return mod;
+    *p_mod = mod;
+    assert(!PyErr_Occurred());
+    return (mod != NULL);
 
 error:
     assert(PyErr_Occurred());
-    return NULL;
+    *p_mod = mod;
+    return -1;
 }
 
 static PyModuleDef datetimemodule;
@@ -164,11 +167,11 @@ static datetime_state *
 _get_current_state(PyObject **p_mod)
 {
     PyInterpreterState *interp = PyInterpreterState_Get();
-    PyObject *mod = get_current_module(interp);
+    PyObject *mod;
+    if (get_current_module(interp, &mod) < 0) {
+        goto error;
+    }
     if (mod == NULL) {
-        if (PyErr_Occurred()) {
-            goto error;
-        }
         /* The static types can outlive the module,
          * so we must re-import the module. */
         mod = PyImport_ImportModule("_datetime");
@@ -7610,8 +7613,8 @@ _datetime_exec(PyObject *module)
     datetime_state *st = get_module_state(module);
 
     PyInterpreterState *interp = PyInterpreterState_Get();
-    PyObject *old_module = get_current_module(interp);
-    if (PyErr_Occurred()) {
+    PyObject *old_module;
+    if (get_current_module(interp, &old_module) < 0) {
         assert(old_module == NULL);
         goto error;
     }

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Removing the PyErr_Occurred() call may make the code a little bit faster. But the new get_current_module() API is also easier to follow.

Comment thread Modules/_datetimemodule.c Outdated

@StanFromIreland StanFromIreland left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with Victor's and my patches.

Comment thread Lib/test/datetimetester.py
@sobolevn sobolevn enabled auto-merge (squash) June 9, 2026 11:17
@sobolevn sobolevn merged commit 9fdbade into python:main Jun 9, 2026
55 checks passed
@miss-islington-app

Copy link
Copy Markdown

Thanks @sobolevn for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14, 3.15.
🐍🍒⛏🤖

@miss-islington-app

Copy link
Copy Markdown

Sorry, @sobolevn, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 9fdbade99e6bcc607d9f12416bfca5bbf94022b9 3.13

@bedevere-app

bedevere-app Bot commented Jun 9, 2026

Copy link
Copy Markdown

GH-151143 is a backport of this pull request to the 3.15 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.15 pre-release feature fixes, bugs and security fixes label Jun 9, 2026
@bedevere-app

bedevere-app Bot commented Jun 9, 2026

Copy link
Copy Markdown

GH-151144 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Jun 9, 2026
sobolevn added a commit that referenced this pull request Jun 9, 2026
…me` module (GH-151044) (#151144)

gh-151039: Fix a crash when `_datetime` types outlive `_datetime` module (GH-151044)
(cherry picked from commit 9fdbade)

Co-authored-by: sobolevn <mail@sobolevn.me>
sobolevn added a commit that referenced this pull request Jun 9, 2026
…me` module (GH-151044) (#151143)

gh-151039: Fix a crash when `_datetime` types outlive `_datetime` module (GH-151044)
(cherry picked from commit 9fdbade)

Co-authored-by: sobolevn <mail@sobolevn.me>
@sobolevn

sobolevn commented Jun 9, 2026

Copy link
Copy Markdown
Member Author

3.13 has a completely different code, I would prefer not to touch it, if possible.

@sobolevn sobolevn removed the needs backport to 3.13 bugs and security fixes label Jun 9, 2026
@vstinner

vstinner commented Jun 9, 2026

Copy link
Copy Markdown
Member

I can reproduce the crash on 3.13. But I'm fine with not backporting the change to 3.13. This _datetime code is quite complicated, and the reproducer does tricky things on the _datetime module which should not occur in regular code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants